Skip to content

perf(heap): look up Gen1/Gen2 membership without the chunk lock - #5409

Closed
harivansh-afk wants to merge 1 commit into
brr/nv0-wb-1-testsfrom
brr/nv0-wb-2-lockfree
Closed

harivansh-afk wants to merge 1 commit into
brr/nv0-wb-1-testsfrom
brr/nv0-wb-2-lockfree

Conversation

@harivansh-afk

Copy link
Copy Markdown
Collaborator

TLDR

Stacked on #5408 (wb-1). Every pointer store runs the write barrier, which calls generation_of on the container and on the value. Each call took the ChunkedVec read lock for Gen2 and then again for Gen1: up to 8 atomic read-modify-writes per store. mark_card_for_ptr took the lock once per chunk scanned. On aarch64 each of these is an outlined __aarch64_cas8_acq / ldadd8_rel call. In native richards they were ~31% of samples, plus 6.8% in generation_of.

The lock only excludes concurrent growth, which Gen1 and Gen2 never see outside a collection.

How

  • ChunkedVec::locate_unsynchronized(ptr) -> Option<(chunk, offset)>: an unsafe scan with no lock. Its contract is that nothing grows, clears, swaps or drops the vector during the call. contains_ptr is now that scan under the read lock.
  • The heap's Gen1/Gen2 lookups (ptr_in_chunked_vec, locate_in_chunked_vec, hence generation_of and mark_card_for_ptr) use the unsynchronized scan.
  • Gen0 grows on any VM's TLAB refill and is never inspected here (a Gen0 pointer is the fallback). Its locked paths are unchanged.

Why it's sound

  • The collector is the only writer of Gen1/Gen2: evacuate_object into gen2, the swap with inactive, and clear. It runs with every mutator parked and is single-threaded, so its own generation_of calls are sequential with its writes.
  • Every barrier caller holds a heap permit, which excludes the collector. Any HeapPtr is only valid under one (heap_guard.rs), and settle_ready / settle_error require it.
  • The code already relied on this: gen2.get() was read through the UnsafeCell with no lock, and generation_of's comment states the invariant. This change only drops a lock that excluded growth that can't happen.

Checks

  • bex_heap tests, with and without heap_debug, including wb-1's matrix. The membership unit test passes under Miri (tree borrows).
  • Full rust_differential, debug assertions on, every mode.
  • TSAN with gc-stress on and fiber switches annotated: 7 spawn/GC probes × bytecode/compiled × 2 runs = 28/28, 0 races. Unannotated TSAN crashes inside its own runtime on compiled Spawns, on feat(compiler): compile BAML MIR to Rust #5264's base too; that is a separate fiber-annotation issue, not this change.

Measurements are in wb-3; the two were benchmarked together.

generation_of runs on every pointer store, through the write barrier. It
took the ChunkedVec read lock for Gen2 and again for Gen1: four atomic
read-modify-writes per lookup, two lookups per store. mark_card_for_ptr took
the lock once per chunk scanned.

The lock only guards concurrent growth, which Gen1 and Gen2 never see
outside a collection: the collector is their only writer and runs with every
mutator parked, and every barrier caller holds a heap permit. Gen0 still
grows on any VM's TLAB refill and keeps the locked contains_ptr.

Add ChunkedVec::locate_unsynchronized, an unsafe scan whose contract is that
nothing changes the vector during the call, and route the heap's Gen1/Gen2
lookups through it.
@coderabbitai

coderabbitai Bot commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption.

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown

⏭️ Performance benchmarks were skipped

Perf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to canary/main.

To run them on this PR, do any of the following, then push a commit (or re-run CI):

  • Add RUN_CODSPEED=1 to the PR description, or
  • Include run-perf or /perf in the PR title or any commit message.

@github-actions

Copy link
Copy Markdown

Binary size checks failed

❌ 2 violations · ✅ 1 passed

⚠️ Please fix the size gate issues or acknowledge them by updating baselines.

Artifact Platform File Gzip Gated on Baseline Delta Status
✅ baml-cli Linux 🔒 64.1 MB 27.3 MB file 73.1 MB -9.0 MB (-12.3%) OK
❌ packed-program Linux 🔒 41.4 MB 16.9 MB file 29.2 MB +12.2 MB (+41.9%) FAIL
❌ bridge_wasm WASM 27.4 MB 🔒 8.0 MB gzip 5.7 MB +2.4 MB (+41.8%) FAIL

🔒 = the size this artifact is GATED on (ceiling + delta). Binaries gate on file size (installed binary); WASM gates on gzip (download size). The other size is shown for information only.

Details & how to fix

Violations:

  • packed-program (Linux) file_bytes: 41.4 MB exceeds limit of 30.1 MB (exceeded by +11.3 MB, policy: max_file_bytes)
  • packed-program (Linux) file_delta_pct: +41.9% exceeds limit of 3.0% (exceeded by +38.9pp, policy: max_delta_pct)
  • bridge_wasm (WASM) gzip_bytes: 8.0 MB exceeds limit of 5.9 MB (exceeded by +2.2 MB, policy: max_gzip_bytes)
  • bridge_wasm (WASM) gzip_delta_pct: +41.8% exceeds limit of 3.0% (exceeded by +38.8pp, policy: max_delta_pct)

Add/update baselines:

.ci/size-gate/wasm32-unknown-unknown.toml:

[artifacts.bridge_wasm]
file_bytes = 27362652
gzip_bytes = 8028539

.ci/size-gate/x86_64-unknown-linux-gnu.toml:

[artifacts.packed-program]
file_bytes = 41364128
gzip_bytes = 16865485

Generated by cargo size-gate · workflow run

@harivansh-afk

Copy link
Copy Markdown
Collaborator Author

Closing: superseded by #5429, which replaced the chunk RwLock with a lock-free append-only directory, so every generation lookup is already lock-free on canary. #5410 does not depend on this and stays open pending a re-benchmark on canary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant